Update application REST APIs and fix multiple bugs#2234
Conversation
Piumal1999
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
|
Warning Review limit reached
More reviews will be available in 46 minutes and 19 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe pull request transforms the Developer Portal API to enforce explicit organization scoping, requiring all application and key-management operations to include an Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
portals/developer-portal/src/controllers/authController.js (2)
224-230: ⚡ Quick winMisleading
awaiton callback-based API.
req.session.save(callback)uses callback style and does not return a Promise. Theawaitresolves immediately toundefinedrather than waiting for the save to complete. The code works correctly because all logic is inside the callback, but theawaitis misleading and could confuse future maintainers.Suggested fix
req.session.returnTo = req.originalUrl; req.session.silentAuthRedirected = true; - await req.session.save((err) => { + req.session.save((err) => { if (err) { logger.error('Session save failed during silent SSO', { error: err.message }); return next(); } passport.authenticate('oauth2', { prompt: 'none' })(req, res, next); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/developer-portal/src/controllers/authController.js` around lines 224 - 230, The await keyword on the callback-based req.session.save() call is misleading because this API does not return a Promise, so await resolves immediately to undefined rather than waiting for the callback to complete. Remove the await keyword from the req.session.save invocation in the silent SSO authentication handler, as the callback pattern already properly handles the asynchronous flow and all logic is correctly contained within the callback.
163-170: 💤 Low valueRedundant header assignment.
Cache-Controlis already set at line 151 before entering the conditional branches. This duplicate set on line 167 is unnecessary.Suggested fix
req.session.destroy((destroyErr) => { if (destroyErr) { logger.error('Session destroy failed on local-auth logout', { error: destroyErr.message }); } - res.set('Cache-Control', 'no-store'); res.redirect(req.originalUrl.replace('/logout', '/login')); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/developer-portal/src/controllers/authController.js` around lines 163 - 170, The Cache-Control header is being set twice in the logout flow: once at line 151 and again inside the req.session.destroy callback at line 167. Remove the duplicate res.set('Cache-Control', 'no-store'); statement from inside the session destroy callback since the header is already configured earlier in the control flow before entering the conditional branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@portals/developer-portal/src/controllers/devportalController.js`:
- Line 165: Fix the typo in the delete success response message in
devportalController.js where "Resouce" is misspelled and should be "Resource".
Update the response text in the res.status(200).send() call to correctly spell
"Resource Deleted Successfully" instead of "Resouce Deleted Successfully". This
typo appears in multiple locations (at lines 165 and 175), so make sure to
correct all occurrences throughout the file.
- Line 75: The request parameters being logged directly in error messages (such
as orgId in req.params.orgId) need to be normalized to remove CR/LF characters
before being attached to log metadata. Extract and normalize the route
parameters (req.params.orgId and any other request-path values) by removing
carriage returns and line feeds early in the request handler, then use these
sanitized values in all subsequent logger.error calls that reference them. This
normalization should be applied consistently across the error logging statements
at lines 75, 90, 109, and 178 to ensure logs remain safe and reliable for
downstream processing.
In `@portals/developer-portal/src/middlewares/authMiddleware.js`:
- Around line 96-131: The checkOrgIsolation function currently treats all errors
from orgDao.get() as internal server errors, but the upstream contract indicates
that a missing organization throws Sequelize.EmptyResultError rather than
returning a falsy value, making the !orgDetails check unreachable. Modify the
catch block that handles the orgDao.get() call to distinguish between
Sequelize.EmptyResultError (which should return a 404 status) and other errors
(which should return 500 status). This will ensure that missing organizations
are properly reported as 404 Not Found instead of 500 Internal Server Error, and
you can remove the now-reachable !orgDetails check.
---
Nitpick comments:
In `@portals/developer-portal/src/controllers/authController.js`:
- Around line 224-230: The await keyword on the callback-based
req.session.save() call is misleading because this API does not return a
Promise, so await resolves immediately to undefined rather than waiting for the
callback to complete. Remove the await keyword from the req.session.save
invocation in the silent SSO authentication handler, as the callback pattern
already properly handles the asynchronous flow and all logic is correctly
contained within the callback.
- Around line 163-170: The Cache-Control header is being set twice in the logout
flow: once at line 151 and again inside the req.session.destroy callback at line
167. Remove the duplicate res.set('Cache-Control', 'no-store'); statement from
inside the session destroy callback since the header is already configured
earlier in the control flow before entering the conditional branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d72555f5-946a-4ca6-b073-a5c622fb9b1b
📒 Files selected for processing (15)
docs/rest-apis/devportal/README.mddocs/rest-apis/devportal/application-keys.mddocs/rest-apis/devportal/applications.mdportals/developer-portal/configs/config.yaml.exampleportals/developer-portal/docs/README.mdportals/developer-portal/docs/administer/api-token-curl.mdportals/developer-portal/docs/administer/asgardeo-setup.mdportals/developer-portal/docs/devportal-openapi-spec-v1.yamlportals/developer-portal/src/controllers/authController.jsportals/developer-portal/src/controllers/devportalController.jsportals/developer-portal/src/middlewares/authMiddleware.jsportals/developer-portal/src/middlewares/ensureAuthenticated.jsportals/developer-portal/src/middlewares/passportConfig.jsportals/developer-portal/src/routes/api/handlers/applicationsHandler.jsportals/developer-portal/src/utils/tokenUtil.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@portals/developer-portal/src/services/apiFlowService.js`:
- Around line 186-192: Move the orgDao.get(orgID) call and
sequelize.transaction() creation from before the try block into inside the try
block so that failures in these operations are caught by the existing catch
handler. Additionally, update the catch block to only perform transaction
rollback when the transaction variable t was successfully created, by checking
if t exists before calling rollback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4aa8be90-e794-4a29-ab76-06fe626b0f8f
📒 Files selected for processing (13)
portals/developer-portal/src/controllers/applicationsContentController.jsportals/developer-portal/src/controllers/authController.jsportals/developer-portal/src/controllers/devportalController.jsportals/developer-portal/src/defaultContent/layout/main.hbsportals/developer-portal/src/middlewares/authMiddleware.jsportals/developer-portal/src/middlewares/ensureAuthenticated.jsportals/developer-portal/src/scripts/add-application-form.jsportals/developer-portal/src/scripts/common.jsportals/developer-portal/src/scripts/edit-application-form.jsportals/developer-portal/src/scripts/oauth2-key-generation.jsportals/developer-portal/src/scripts/warning.jsportals/developer-portal/src/services/apiFlowService.jsportals/developer-portal/src/services/apiMetadataService.js
💤 Files with no reviewable changes (1)
- portals/developer-portal/src/services/apiMetadataService.js
🚧 Files skipped from review as they are similar to previous changes (4)
- portals/developer-portal/src/middlewares/authMiddleware.js
- portals/developer-portal/src/controllers/devportalController.js
- portals/developer-portal/src/middlewares/ensureAuthenticated.js
- portals/developer-portal/src/controllers/authController.js
…n handling and scope management
…ganization isolation checks
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/rest-apis/devportal/applications.md (1)
363-363:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate Delete operation description for consistency with other operations.
The Delete operation description omits reference to organization scoping, unlike the List, Create, and Update operations. Since the endpoint requires
orgIdas a path parameter, the description should clarify that deletion occurs within the specified organization.Suggested change: "Deletes an application owned by the authenticated user in the specified organization."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/rest-apis/devportal/applications.md` at line 363, The Delete operation description for applications at line 363 omits reference to organization scoping, which is inconsistent with the List, Create, and Update operations that all include organization context. Update the Delete operation description to clarify that the deletion occurs within the specified organization. Add "in the specified organization" or similar phrasing to match the organization scoping pattern used in the other operation descriptions, since the endpoint requires orgId as a path parameter.
🧹 Nitpick comments (1)
portals/developer-portal/src/dao/applicationDao.js (1)
57-61: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding null guard for the findOne result.
If the record is deleted between the successful update and the subsequent
findOne,updatedAppwill benull. The controller accessesupdatedApp[0].dataValues, which would throw an error in this unlikely scenario.Additionally, for consistency with the
getfunction (line 77) and the update WHERE clause itself, consider includingCREATED_BY: userIDin the findOne query.Suggested change
if (!updatedRowsCount) { return [updatedRowsCount, null]; } - const updatedApp = await Application.findOne({ where: { ORG_ID: orgID, APP_ID: appID } }); + const updatedApp = await Application.findOne({ where: { ORG_ID: orgID, APP_ID: appID, CREATED_BY: userID } }); + if (!updatedApp) { + return [updatedRowsCount, null]; + } return [updatedRowsCount, [updatedApp]];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@portals/developer-portal/src/dao/applicationDao.js` around lines 57 - 61, Add a null guard after the Application.findOne call in the update function to handle the edge case where the record might be deleted between the successful update and the subsequent query. Check if updatedApp is null and return an appropriate response (such as null or an error indicator) to prevent the controller from accessing updatedApp[0].dataValues on a null value. Additionally, include CREATED_BY: userID in the findOne WHERE clause alongside ORG_ID and APP_ID to maintain consistency with the get function and the WHERE clause used in the update operation itself.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/rest-apis/devportal/applications.md`:
- Line 363: The Delete operation description for applications at line 363 omits
reference to organization scoping, which is inconsistent with the List, Create,
and Update operations that all include organization context. Update the Delete
operation description to clarify that the deletion occurs within the specified
organization. Add "in the specified organization" or similar phrasing to match
the organization scoping pattern used in the other operation descriptions, since
the endpoint requires orgId as a path parameter.
---
Nitpick comments:
In `@portals/developer-portal/src/dao/applicationDao.js`:
- Around line 57-61: Add a null guard after the Application.findOne call in the
update function to handle the edge case where the record might be deleted
between the successful update and the subsequent query. Check if updatedApp is
null and return an appropriate response (such as null or an error indicator) to
prevent the controller from accessing updatedApp[0].dataValues on a null value.
Additionally, include CREATED_BY: userID in the findOne WHERE clause alongside
ORG_ID and APP_ID to maintain consistency with the get function and the WHERE
clause used in the update operation itself.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bce2ed5-1171-4a0c-b347-ef52fc3d49dc
📒 Files selected for processing (27)
docs/rest-apis/devportal/README.mddocs/rest-apis/devportal/application-keys.mddocs/rest-apis/devportal/applications.mdportals/developer-portal/configs/config.yaml.exampleportals/developer-portal/docs/README.mdportals/developer-portal/docs/administer/api-token-curl.mdportals/developer-portal/docs/administer/asgardeo-setup.mdportals/developer-portal/docs/devportal-openapi-spec-v1.yamlportals/developer-portal/src/controllers/applicationsContentController.jsportals/developer-portal/src/controllers/authController.jsportals/developer-portal/src/controllers/devportalController.jsportals/developer-portal/src/dao/applicationDao.jsportals/developer-portal/src/defaultContent/layout/main.hbsportals/developer-portal/src/middlewares/authMiddleware.jsportals/developer-portal/src/middlewares/ensureAuthenticated.jsportals/developer-portal/src/middlewares/passportConfig.jsportals/developer-portal/src/pages/applications/partials/applications-listing.hbsportals/developer-portal/src/routes/api/handlers/applicationsHandler.jsportals/developer-portal/src/scripts/add-application-form.jsportals/developer-portal/src/scripts/common.jsportals/developer-portal/src/scripts/edit-application-form.jsportals/developer-portal/src/scripts/oauth2-key-generation.jsportals/developer-portal/src/scripts/warning.jsportals/developer-portal/src/services/apiFlowService.jsportals/developer-portal/src/services/apiMetadataService.jsportals/developer-portal/src/styles/applications.cssportals/developer-portal/src/utils/tokenUtil.js
✅ Files skipped from review due to trivial changes (7)
- portals/developer-portal/docs/README.md
- docs/rest-apis/devportal/README.md
- portals/developer-portal/src/styles/applications.css
- portals/developer-portal/src/services/apiMetadataService.js
- portals/developer-portal/src/controllers/applicationsContentController.js
- portals/developer-portal/docs/administer/api-token-curl.md
- docs/rest-apis/devportal/application-keys.md
🚧 Files skipped from review as they are similar to previous changes (16)
- portals/developer-portal/src/defaultContent/layout/main.hbs
- portals/developer-portal/src/routes/api/handlers/applicationsHandler.js
- portals/developer-portal/src/middlewares/passportConfig.js
- portals/developer-portal/src/scripts/add-application-form.js
- portals/developer-portal/src/scripts/common.js
- portals/developer-portal/src/scripts/warning.js
- portals/developer-portal/src/scripts/edit-application-form.js
- portals/developer-portal/configs/config.yaml.example
- portals/developer-portal/src/utils/tokenUtil.js
- portals/developer-portal/src/services/apiFlowService.js
- portals/developer-portal/docs/administer/asgardeo-setup.md
- portals/developer-portal/src/controllers/devportalController.js
- portals/developer-portal/src/controllers/authController.js
- portals/developer-portal/docs/devportal-openapi-spec-v1.yaml
- portals/developer-portal/src/middlewares/authMiddleware.js
- portals/developer-portal/src/middlewares/ensureAuthenticated.js
2 participants
Purpose
Correcting the application related rest APIs
Updating docs
Fixing multiple authentication related bugs
Approach
o/{orgId}prefix similar to other APIs